-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Routine maintenance 2023-08 #596
Conversation
@@ -23,9 +23,13 @@ function DropdownSelect({ children, className, selectedValues }) { | |||
return ( | |||
<OutsideClickHandler onOutsideClick={() => toggleExpanded(false)}> | |||
<div className="dropdown-select"> | |||
<div className="select-input" onClick={() => toggleExpanded()}> | |||
<button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagged by the jsx-a11y
plugin from upgrading lpl's eslint-config
* - `errorComponent` | ||
* - `labelComponent` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed these were missing from the docs
previewComponent: PropTypes.func, // eslint-disable-line react/no-unused-prop-types | ||
children: PropTypes.node, // eslint-disable-line react/no-unused-prop-types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We know these are being used by the RenderPreview component
@@ -44,7 +44,7 @@ import { compose, generateInputErrorId } from '../../utils' | |||
|
|||
const propTypes = { | |||
...fieldPropTypesWithValue(PropTypes.bool), | |||
label: PropTypes.node, | |||
label: PropTypes.node, // eslint-disable-line react/no-unused-prop-types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We know this is getting passed to LabeledField
@bhennes2 I didn't originally intend for this to be a breaking change, but once I upgraded to our latest eslint configuration there were a few |
test('ColorPicker collapses when background is clicked', () => { | ||
const wrapper = mount(<ColorPicker />) | ||
wrapper.find('.swatch').simulate('click') | ||
expect(wrapper.find('.popover').exists()).toBe(true) | ||
wrapper.find('.cover').simulate('click') | ||
expect(wrapper.find('.popover').exists()).toBe(false) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enzyme's click
simulation doesn't trigger the OnOutsideClick
component's logic. I'm ok with that, given that we're using a third party library which has its own test coverage
@bhennes2 Bump 🤜 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looking good - have a few follow up questions on the omitLabelProps
changes just the make sure I'm understanding the diff between props
and rest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we cleared up my remaining questions so going to re-review and approve!
QA
yarn lint
runs with 0 warnings and 0 errorsOut of Scope
Author Checklist